feat(sql_execution): Implement retry logic for userpod API#89
feat(sql_execution): Implement retry logic for userpod API#89
Conversation
- Added a new function `_create_retry_session` to create a requests session with retry capabilities for handling 5xx errors on POST requests. - Updated `_generate_temporary_credentials` and `_get_federated_auth_credentials` to use the new retry session for making requests. - Introduced unit tests to verify the retry session configuration and its usage in the credential generation functions.
📝 WalkthroughWalkthroughAdded Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Creator as _create_retry_session()
participant Session as RetrySession
participant Auth as Auth Server
Caller->>Creator: request retry-enabled session
Creator-->>Caller: configured requests.Session
Caller->>Session: session.post(url, headers, timeout=10)
Session->>Auth: HTTP POST (may retry on 500/502/503/504)
Auth-->>Session: HTTP response(s)
Session-->>Caller: response object
Caller->>Caller: response.raise_for_status() and parse JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
📦 Python package built successfully!
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepnote_toolkit/sql/sql_execution.py`:
- Around line 267-278: The cache lookup in _request_cache_info_from_webapp is
using requests.get directly and thus misses the retry behavior defined in
_create_retry_session; modify _create_retry_session (or its Retry config) to
include GET in allowed_methods (or use allowed_methods=None to cover all
idempotent methods) and then replace the direct requests.get call inside
_request_cache_info_from_webapp with a call through a session returned by
_create_retry_session (e.g., session.get(...)) so cache lookups get the same 5xx
retry protection as POSTs.
- Line 281: Update the signature of
_generate_temporary_credentials(integration_id) to include an explicit return
type annotation (e.g., -> Dict[str, Any]) and add the corresponding typing
imports (from typing import Dict, Any) at the top of the module; ensure the
annotated type matches the actual returned structure from
_generate_temporary_credentials so the function signature and implementation
remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bf6b45a7-61ec-4a87-a583-c7d418b8fe72
📒 Files selected for processing (2)
deepnote_toolkit/sql/sql_execution.pytests/unit/test_sql_execution.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
+ Coverage 74.32% 74.45% +0.13%
==========================================
Files 94 94
Lines 5534 5543 +9
Branches 824 824
==========================================
+ Hits 4113 4127 +14
+ Misses 1155 1150 -5
Partials 266 266
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
🚀 Review App Deployment Started
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_sql_execution.py`:
- Around line 1055-1065: Convert the context-manager style mocking of
deepnote_toolkit.sql.sql_execution._create_retry_session (currently using with
mock.patch(...) as mock_create_session in the test block) to the decorator style
`@mock.patch` on the test function signature (e.g., add
`@mock.patch`("deepnote_toolkit.sql.sql_execution._create_retry_session") and a
mock_create_session parameter) and remove the inner with-block; apply the same
change to the similar block around lines 1089-1099 so both tests use
decorator-style mocking for consistency with
test_get_federated_auth_credentials_returns_validated_response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 469a19f4-668a-4b55-8dfc-c8551fc61827
📒 Files selected for processing (1)
tests/unit/test_sql_execution.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
deepnote_toolkit/sql/sql_execution.py (1)
280-280:⚠️ Potential issue | 🟡 MinorAdd parameter type hint.
Return type added, but
integration_idlacks type annotation per coding guidelines.🔧 Fix
-def _generate_temporary_credentials(integration_id) -> tuple[str, str]: +def _generate_temporary_credentials(integration_id: str) -> tuple[str, str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepnote_toolkit/sql/sql_execution.py` at line 280, The function _generate_temporary_credentials has a return type but the parameter integration_id is missing a type annotation; update the function signature for _generate_temporary_credentials to add the appropriate type (e.g., integration_id: str) to comply with the coding guidelines and any static type checks, keeping the return annotation tuple[str, str] unchanged and adjusting any callers only if they rely on a different type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepnote_toolkit/sql/sql_execution.py`:
- Around line 270-274: The Retry instance named "retries" is missing
allowed_methods so POST requests won't be retried; update the Retry(...) call
(the retries variable in sql_execution.py) to include an allowed_methods
parameter that includes "POST" (for example
allowed_methods=frozenset({"GET","POST","PUT","DELETE","HEAD","OPTIONS","TRACE"})
or at minimum allowed_methods=frozenset({"POST"}) depending on desired scope) so
POST requests are retried per the backoff/status_forcelist settings.
In `@tests/unit/test_sql_execution.py`:
- Around line 1026-1040: The Retry configuration in _create_retry_session is
missing the allowed_methods parameter so POST retries aren't enabled; update the
Retry(...) construction inside the _create_retry_session function to include
allowed_methods=["DELETE","GET","HEAD","OPTIONS","POST","PUT","TRACE"] (or an
equivalent set that includes "POST"), then ensure that same Retry instance is
used when mounting the HTTP/HTTPS adapters so
adapter.max_retries.allowed_methods contains "POST".
---
Duplicate comments:
In `@deepnote_toolkit/sql/sql_execution.py`:
- Line 280: The function _generate_temporary_credentials has a return type but
the parameter integration_id is missing a type annotation; update the function
signature for _generate_temporary_credentials to add the appropriate type (e.g.,
integration_id: str) to comply with the coding guidelines and any static type
checks, keeping the return annotation tuple[str, str] unchanged and adjusting
any callers only if they rely on a different type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db1a3ece-0680-4ebe-a62e-d27e219521b2
📒 Files selected for processing (2)
deepnote_toolkit/sql/sql_execution.pytests/unit/test_sql_execution.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deepnote_toolkit/sql/sql_execution.py (1)
281-281:⚠️ Potential issue | 🟡 MinorMissing type hint for
integration_id.Per coding guidelines, explicit type hints are required for parameters.
Proposed fix
-def _generate_temporary_credentials(integration_id) -> tuple[str, str]: +def _generate_temporary_credentials(integration_id: str) -> tuple[str, str]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepnote_toolkit/sql/sql_execution.py` at line 281, The function _generate_temporary_credentials is missing a parameter type hint for integration_id; update the signature to include an explicit type (e.g., integration_id: str) so it becomes def _generate_temporary_credentials(integration_id: str) -> tuple[str, str]: and adjust any callers or tests if they rely on a different type to keep annotations consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_sql_execution.py`:
- Around line 1026-1092: Add a unit test for _create_retry_session that does not
mock it so you can inspect the session object's retry configuration; call
deepnote_toolkit.sql.sql_execution._create_retry_session(), get the returned
requests.Session, locate its mounted HTTPAdapter (e.g.,
session.get_adapter("https://")), and assert the adapter's max_retries
(requests.packages.urllib3.util.retry.Retry or similar) has total==3,
backoff_factor==0.5, status_forcelist contains the expected status codes, and
allowed_methods includes the expected idempotent methods.
---
Duplicate comments:
In `@deepnote_toolkit/sql/sql_execution.py`:
- Line 281: The function _generate_temporary_credentials is missing a parameter
type hint for integration_id; update the signature to include an explicit type
(e.g., integration_id: str) so it becomes def
_generate_temporary_credentials(integration_id: str) -> tuple[str, str]: and
adjust any callers or tests if they rely on a different type to keep annotations
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9bf096b-e2bf-4305-8062-6bbd7168cd64
📒 Files selected for processing (2)
deepnote_toolkit/sql/sql_execution.pytests/unit/test_sql_execution.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_sql_execution.py`:
- Around line 1226-1261: The test
test_federated_auth_exhausts_retries_on_persistent_5xx declares mock_retry_sleep
but never asserts it; update this test to assert that mock_retry_sleep was
invoked the expected number of times (3 sleeps for 4 failed attempts) after
calling _get_federated_auth_credentials("test-id", "auth-context-token") and
after the RetryError assertion; reference the mock object name mock_retry_sleep
and the test function test_federated_auth_exhausts_retries_on_persistent_5xx
when adding mock_retry_sleep.assertEqual(mock_retry_sleep.call_count, 3) (or
equivalent assert_called_count).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 414980d6-b46e-4a59-bebc-7b7b56420428
📒 Files selected for processing (1)
tests/unit/test_sql_execution.py
| @mock.patch("urllib3.util.retry.Retry.sleep", return_value=None) | ||
| @mock.patch("urllib3.connectionpool.HTTPConnectionPool._make_request") | ||
| @mock.patch("deepnote_toolkit.sql.sql_execution.get_project_auth_headers") | ||
| @mock.patch("deepnote_toolkit.sql.sql_execution.get_absolute_userpod_api_url") | ||
| def test_federated_auth_exhausts_retries_on_persistent_5xx( | ||
| self, | ||
| mock_get_url, | ||
| mock_get_headers, | ||
| mock_make_request, | ||
| mock_retry_sleep, | ||
| ): | ||
| """All 4 attempts return 504 - must raise ``RetryError``.""" | ||
| import requests | ||
| from urllib3 import HTTPResponse as Urllib3Response | ||
|
|
||
| from deepnote_toolkit.sql.sql_execution import _get_federated_auth_credentials | ||
|
|
||
| mock_get_url.return_value = ( | ||
| "https://api.example.com/integrations/federated-auth-token/test-id" | ||
| ) | ||
| mock_get_headers.return_value = {"Authorization": "Bearer token"} | ||
|
|
||
| mock_make_request.side_effect = [ | ||
| Urllib3Response( | ||
| body=io.BytesIO(b"Gateway Timeout"), | ||
| status=504, | ||
| headers={}, | ||
| preload_content=False, | ||
| ) | ||
| for _ in range(4) | ||
| ] | ||
|
|
||
| with self.assertRaises(requests.exceptions.RetryError): | ||
| _get_federated_auth_credentials("test-id", "auth-context-token") | ||
|
|
||
| self.assertEqual(mock_make_request.call_count, 4) |
There was a problem hiding this comment.
Missing assertion on mock_retry_sleep.
mock_retry_sleep is declared but unused. The parallel test for _generate_temporary_credentials asserts mock_retry_sleep.call_count == 3. Add the same assertion here.
🔧 Proposed fix
with self.assertRaises(requests.exceptions.RetryError):
_get_federated_auth_credentials("test-id", "auth-context-token")
self.assertEqual(mock_make_request.call_count, 4)
+ self.assertEqual(mock_retry_sleep.call_count, 3)🧰 Tools
🪛 Ruff (0.15.9)
[warning] 1235-1235: Unused method argument: mock_retry_sleep
(ARG002)
[warning] 1258-1258: Use pytest.raises instead of unittest-style assertRaises
Replace assertRaises with pytest.raises
(PT027)
[warning] 1261-1261: Use a regular assert instead of unittest-style assertEqual
Replace assertEqual(...) with assert ...
(PT009)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_sql_execution.py` around lines 1226 - 1261, The test
test_federated_auth_exhausts_retries_on_persistent_5xx declares mock_retry_sleep
but never asserts it; update this test to assert that mock_retry_sleep was
invoked the expected number of times (3 sleeps for 4 failed attempts) after
calling _get_federated_auth_credentials("test-id", "auth-context-token") and
after the RetryError assertion; reference the mock object name mock_retry_sleep
and the test function test_federated_auth_exhausts_retries_on_persistent_5xx
when adding mock_retry_sleep.assertEqual(mock_retry_sleep.call_count, 3) (or
equivalent assert_called_count).
_create_retry_sessionto create a requests session with retry capabilities for handling 5xx errors on POST requests._generate_temporary_credentialsand_get_federated_auth_credentialsto use the new retry session for making requests.Summary by CodeRabbit
New Features
Tests